-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Upgrade ginkgo to v2 #2522
✨ Upgrade ginkgo to v2 #2522
Conversation
Welcome @haoxins! |
Hi @haoxins. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: haoxins The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/ok-to-test |
pkg/plugins/golang/v2/scaffolds/internal/templates/controllers/controller_suitetest.go
Outdated
Show resolved
Hide resolved
@@ -104,9 +104,7 @@ var _ = BeforeSuite(func(done Done) { | |||
|
|||
_, err := testenv.Start() | |||
Expect(err).NotTo(HaveOccurred()) | |||
|
|||
close(done) | |||
}, 60) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the done was removed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shows that the migration for this one is:
It("...", func() {
done := make(chan interface{})
go func() {
// user test code to run asynchronously
close(done) //signifies the code is done
}()
Eventually(done, timeout).Should(BeClosed())
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, if async
is important for us, we can do that.
Also, custom reports~
. "github.com/onsi/gomega" | ||
"k8s.io/client-go/kubernetes/scheme" | ||
"k8s.io/client-go/rest" | ||
"sigs.k8s.io/controller-runtime/pkg/client" | ||
"sigs.k8s.io/controller-runtime/pkg/envtest" | ||
"sigs.k8s.io/controller-runtime/pkg/envtest/printer" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the printer was removed?
@@ -44,9 +43,7 @@ var testEnv *envtest.Environment | |||
func TestAPIs(t *testing.T) { | |||
RegisterFailHandler(Fail) | |||
|
|||
RunSpecsWithDefaultAndCustomReporters(t, | |||
"Controller Suite", | |||
[]Reporter{printer.NewlineReporter{}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we are changing the default scaffold more than just the import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, how could we achieve the same with the v2? Are we able to provide it via the scaffolds? How will it impact the author's projects?
Also, it shows a breaking change for authors' projects so the emoji needs to be changed to warning
:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the migration for this involves adding a ReportAfterSuite
node: https://onsi.github.io/ginkgo/MIGRATING_TO_V2#generating-custom-reports-when-a-test-suite-completes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we need to. change the scaffold then it shows a breaking change.
testdata/project-v3-v1beta1/go.mod
Outdated
github.com/onsi/ginkgo v1.16.4 | ||
github.com/onsi/gomega v1.13.0 | ||
github.com/onsi/ginkgo/v2 v2.1.3 | ||
github.com/onsi/gomega v1.17.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version is probably incompatible. We need to revert the dep if/when the v1beta1 be used see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/v3/commons.go#L38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nit changes, mainly
- Ensure that we are updating the dep but does not change the default scaffold implementation
- Ensure that the update is valid only for the go/v3 plugin and we will do the downgrade if/when v1beta1 CRD be a scaffold for backwards compatibilities.
Also, see:
- The title/commit is used to create the release notes. then, this change affects the scaffold so we need to use the emoji
:sparkles:
. More info: https://github.com/kubernetes-sigs/kubebuilder/blob/master/CONTRIBUTING.md#pr-process- WDYT about : (go/v3) - Upgrade dep from github.com/onsi/ginkgo to github.com/onsi/ginkgo/v2
Maybe I can split the scaffold change into another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamacedo86 I might be totally missing something, but what does the ginkgo version have to do with the CRD version? I would expect ginkgo/v2 to work with projects that use v1beta1 CRDs. Is it just that the other dependencies we're required to use with v1beta1 CRDs use ginkgo/v1, so we're concerned that there would be issues if the kubebuilder project used v2, but the project's dependencies used v1?
The one important thing to cover is this:
- Regardless of the CRD version (or anything else), if a project was originally scaffolded with ginkgo v1, we should continue scaffolding new APIs and webhooks with ginkgo v1.
I'm not sure I saw anything for that in my review, but if we have that covered, disregard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we scaffold CRD v1beta1 we need to ensure that we use compatible deps with controller-gen v0.6.2 and controller-runtime v0.9.2 since these are the latest versions where we can use v1beta1 with. So, my point is that:
- We need to check if the ginkgo v2 is compatible with or not, if not we need to try to downgrade (here)
- However, we checked in the review that changes in the scaffolds are required and that the previous code will not work with ginkgov2 so it is a breaking change and then we cannot move forward in go/v3. We will need a go/v4 plugin to add this one. I am thinking that we should track it to address with go/v4, see: https://github.com/kubernetes-sigs/kubebuilder/milestone/26
@joelanford wdyt?
/hold
(it shows a breaking change and cannot be addressed on go/v3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nit changes, mainly
- Ensure that we are updating the dep but does not change the default scaffold implementation
- Ensure that the update is valid only for the go/v3 plugin and we will do the downgrade if/when v1beta1 CRD be a scaffold for backwards compatibilities.
Also, see:
- The title/commit is used to create the release notes. then, this change affects the scaffold so we need to use the emoji
:sparkles:
. More info: https://github.com/kubernetes-sigs/kubebuilder/blob/master/CONTRIBUTING.md#pr-process - WDYT about : (go/v3) - Upgrade dep from github.com/onsi/ginkgo to github.com/onsi/ginkgo/v2
@camilamacedo86 I just reverted the change to scaffold so that I can finish the |
Updated~ |
revert useless change revert chore change v3 scaffolds base on CRD version fix fix fix apply fix
@haoxins: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @haoxins,
We checked in the review that changes in the scaffolds are required and that the previous code will not work with ginkgo v2 so it is a breaking change and then we cannot move forward in go/v3. We will need a go/v4 plugin to add this one. I am thinking that we should track it to address with go/v4, see: https://github.com/kubernetes-sigs/kubebuilder/milestone/26. WDYT Could you help us out by doing that and linking this PR?
See:
- https://onsi.github.io/ginkgo/MIGRATING_TO_V2#generating-custom-reports-when-a-test-suite-completes
- https://onsi.github.io/ginkgo/MIGRATING_TO_V2#removed-async-testing
Also, you can check the policy: https://github.com/kubernetes-sigs/kubebuilder/blob/master/VERSIONING.md#introducing-changes-to-plugins
@joelanford wdyt? Have you objections to it?
|
Hi @haoxins, I am closing this one since we can plan that for go/v4 and we could check that is a breaking change and I hope that you do not mind. Thank you it will be helpful for we achieve it with go/v4 as well. |
No description provided.